Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash with generic class definition in function #13678

Merged

Conversation

Michael0x2a
Copy link
Collaborator

Fixes #12112.

The reason why mypy was crashing with a "Must not defer during final iteration" error in the following snippet:

from typing import TypeVar

def test() -> None:
    T = TypeVar('T', bound='Foo')

    class Foo:
        def bar(self, foo: T) -> None:
            pass

...was because mypy did not seem to be updating the types of the bar callable on each pass: the bind_function_type_variables method in typeanal.py always returned the old type variables instead of using the new updated ones we found by calling self.lookup_qualified(...).

This in turn prevented us from making any forward progress when mypy generated a CallableType containing a placedholder type variable. So, we repeated the semanal passes until we hit the limit and crashed.

I opted to fix this by having the function always return the newly-bound TypeVarLikeType instead. (Hopefully this is safe -- the way mypy likes mutating types always makes it hard to reason about this sort of stuff).

Interestingly, my fix for this bug introduced a regression in one of our existing tests:

from typing import NamedTuple, TypeVar

T = TypeVar("T")
NT = NamedTuple("NT", [("key", int), ("value", T)])

# Test thinks the revealed type should be:
#     def [T] (key: builtins.int, value: T`-1) -> Tuple[builtins.int, T`-1, fallback=__main__.NT[T`-1]]
#
# ...but we started seeing:
#     def [T, _NT <: Tuple[builtins.int, T`-1]] (key: builtins.int, value: T`-1) -> Tuple[builtins.int, T`-1, fallback=test.WTF[T`-1]]
reveal_type(NT)

What seems to be happening here is that during the first pass, we add two type vars to the tvar_scope inside bind_function_type_variables: T with id -1 and _NT with id -2.

But in the second pass, we lose track of the T typevar definition and/or introduce a fresh scope somewhere and infer _NT with id -1 instead?

So now mypy thinks there are two type variables associated with this NamedTuple, which results in the screwed-up type definition.

I wasn't really sure how to fix this, but I thought it was weird that:

  1. We were using negative IDs instead of positive ones. (Class typevars are supposed to use the latter).
  2. We weren't wrapping this whole thing in a new tvar scope frame, given we're nominally synthesizing a new class.

So I did that, and the tests started passing?

I wasn't able to repro this issue for TypedDicts, but opted to introduce a new tvar scope frame there as well for consistency.

Fixes python#12112.

The reason why mypy was crashing with a "Must not defer during final
iteration" error in the following snippet:

    from typing import TypeVar

    def test() -> None:
        T = TypeVar('T', bound='Foo')

        class Foo:
            def bar(self, foo: T) -> None:
                pass

...was because mypy did not seem to be updating the types of the `bar`
callable on each pass: the `bind_function_type_variables` method in
`typeanal.py` always returned the _old_ type variables instead of using
the new updated ones we found by calling `self.lookup_qualified(...)`.

This in turn prevented us from making any forward progress when mypy
generated a CallableType containing a placedholder type variable. So,
we repeated the semanal passes until we hit the limit and crashed.

I opted to fix this by having the function always return the newly-bound
TypeVarLikeType instead. (Hopefully this is safe -- the way mypy likes
mutating types always makes it hard to reason about this sort of stuff).

Interestingly, my fix for this bug introduced a regression in one of
our existing tests:

    from typing import NamedTuple, TypeVar

    T = TypeVar("T")
    NT = NamedTuple("NT", [("key", int), ("value", T)])

    # Test thinks the revealed type should be:
    #     def [T] (key: builtins.int, value: T`-1) -> Tuple[builtins.int, T`-1, fallback=__main__.NT[T`-1]]
    #
    # ...but we started seeing:
    #     def [T, _NT <: Tuple[builtins.int, T`-1]] (key: builtins.int, value: T`-1) -> Tuple[builtins.int, T`-1, fallback=test.WTF[T`-1]]
    reveal_type(NT)

What seems to be happening here is that during the first pass, we add
two type vars to the `tvar_scope` inside `bind_function_type_variables`:
`T` with id -1 and `_NT` with id -2.

But in the second pass, we lose track of the `T` typevar definition and/or
introduce a fresh scope somewhere and infer `_NT` with id -1 instead?

So now mypy thinks there are two type variables associated with this
NamedTuple, which results in the screwed-up type definition.

I wasn't really sure how to fix this, but I thought it was weird that:

1. We were using negative IDs instead of positive ones. The former is
   meant to be for class definitions, which is what we're using here.
2. We weren't wrapping this whole thing in a new tvar scope frame, given
   we're nominally synthesizing a new class.

So I did that, and the tests started passing?

I wasn't able to repro this issue for TypedDicts, but opted to introduce
a new tvar scope frame there as well for consistency.
@github-actions

This comment has been minimized.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was quite a tricky bug. Thanks for fixing this! Looks good, left some optional ideas about tests.

T = TypeVar('T', bound='Foo')
class Foo:
def bar(self, foo: T) -> T:
pass
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using T in the function body (e.g. x: T = foo, reveal_type(x)), just in case?

Would it make sense to test using T in the class body -- this should generate an error, since the binding is out of scope. This should probably be a separate test case (or at least a separate function in this test case) to avoid interfering with the current test case.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@Michael0x2a Michael0x2a merged commit 5094460 into python:master Sep 25, 2022
@Michael0x2a Michael0x2a deleted the fix-crash-with-nested-generic-class branch September 25, 2022 21:05
Michael0x2a added a commit to Michael0x2a/mypy that referenced this pull request Sep 29, 2022
This is a small follow-up to python#13678 and python#13755.

In the former diff, I added a TypeVar scope frame around
TypedDict creation. However, I had really no good reason for
doing this at the time: I wasn't able to find a bug due to
the missing frame and added it purely speculatively, out of a
desire for symmetry.

It turns out this missing frame does legitimately cause
some issues, which were reported in the latter.

So, this diff encodes one of the reported bugs as a test
case to make sure we don't regress.
hauntsaninja pushed a commit that referenced this pull request Sep 30, 2022
This is a small follow-up to #13678 and #13755.

In the former diff, I added a TypeVar scope frame around TypedDict
creation. However, I had really no good reason for doing this at the
time: I wasn't able to find a bug due to the missing frame and added it
purely speculatively, out of a desire for symmetry.

It turns out this missing frame does legitimately cause some issues,
which were reported in the latter.

So, this diff encodes one of the reported bugs as a test case to make
sure we don't regress.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mixin class factory crash
2 participants